Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

updateAttributes: update current node only #5154

Merged
merged 13 commits into from
Jun 7, 2024
Merged

updateAttributes: update current node only #5154

merged 13 commits into from
Jun 7, 2024

Conversation

silenius
Copy link
Contributor

@silenius silenius commented May 15, 2024 •
edited
Loading

There is a bug (?) in updateAttributes, attributes of the node are updated but also all it's parent nodes of the same type. This is problematic with things like nested lists, but also nested paragraphs, etc

The fix updates attributes of the node ("last") only and leaves the parents intact

Changes Overview

updateAttributes update node only (and not its parents)

Implementation Approach

adapt updateAttributes so that it keep tracks of the "last" (current) node

Testing Done

tested locally with bulletList nodes

Verification Steps

try to update an attributes of a nested list for examplectionality of your changes. -->

Checklist

  • I have renamed my PR according to the naming conventions. (e.g. feat: Implement new feature or chore(deps): Update dependencies)
  • My changes do not break the library.
  • I have added tests where applicable.
  • I have followed the project guidelines.
  • I have fixed any lint issues.

Related Issues

#3545 #4466

@silenius
updateAttributes: update current node only
ca441ef 
There is a bug (?) in updateAttributes, attributes of the node are updated
but also all it's parent nodes of the same type. This is problematic with
things like nested lists, but also nested paragraphs, etc

The fix updates attributes of the node ("last") only and leaves the
parents intact
@silenius silenius requested review from bdbch and svenadlung as code owners May 15, 2024 10:20
@netlify Netlify
Copy link

netlify bot commented May 15, 2024 •
edited
Loading

✅ Deploy Preview for tiptap-embed ready!

Name Link
🔨 Latest commit cc25b15
🔍 Latest deploy log https://app.netlify.com/sites/tiptap-embed/deploys/664ba85e8ba9640009aed7dc
😎 Deploy Preview https://deploy-preview-5154--tiptap-embed.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@nperez0111
Copy link

nperez0111 commented May 17, 2024 •
edited
Loading

Appreciate your contribution @silenius but it looks like we need some tests & lint rules resolved for this to be merged

@silenius
Revert "Merge branch 'main' into updateAttributes"
359b309 
This reverts commit 8a16bbe, reversing
changes made to 90a5a7b.
@silenius
Copy link
Contributor Author

Sorry, I pressed the wrong button (:

@nperez0111
Copy link

I don't think that is correct either. You were right to pull in the changes from main, it is your changes that are failing the build
@silenius

@silenius
Copy link
Contributor Author

I've fixed the lint issue, but I have no idea why it times out in one test..? (unfortunately Cypress is not supported on my OS, FreeBSD, so I can't check at the moment)

@nperez0111
Copy link

@silenius if you run the project locally npm run dev you'll see that when you go to: http://localhost:3000/src/Extensions/TextAlign/React/
that if you clear the editor completely, add some new text into the editor (which is what the test does on each run)
then select the content and click the center button that it does not work.

This is also seen on the netlify deployment: https://deploy-preview-5154--tiptap-embed.netlify.app/src/extensions/textalign/react/

So your change broke the ability to center text with the text align extension somehow

Hope this helps @silenius

@nperez0111 nperez0111 added Type: Bug The issue or pullrequest is related to a bug Info: Invalid The issue or pullrequest is invalid labels May 17, 2024
@silenius
Copy link
Contributor Author

thanks, it should be fixed :)

@silenius silenius requested a review from nperez0111 May 21, 2024 09:12
silenius added a commit to silenius/amnesia_admin that referenced this pull request Jun 4, 2024
@silenius
add custom updateAttributes (see ueberdosis/tiptap#5154)
ba7aa91
@nperez0111 nperez0111 merged commit a52118c into ueberdosis:main Jun 7, 2024
14 checks passed
@nperez0111
Copy link

Appreciate your contribution here, it will go into the next release. Might take a while, so I will make a beta that includes it soon

@svilen-ivanov-kubit svilen-ivanov-kubit mentioned this pull request Jun 19, 2024
Closed
1 task
@svilen-ivanov-kubit
Copy link

Hi, I think this change caused the following problem: #5254

@nperez0111
Copy link

Hi, I think this change caused the following problem: #5254

Thanks for your feedback. I will write a test for this & see if I can resolve it or revert it

@nperez0111
Copy link

Sorry @silenius I did not get around to handling this. If you have an idea on how to support this, that would very much be appreciated. I'm trying to get this new release out and so I had to revert it

@silenius
Copy link
Contributor Author

Hi @nperez0111,

Sorry for breakage, I'll try to find a little bit of time to review it.. but it could be reverted in the meantime

@nperez0111
Copy link

Thanks for your contribution in the first place!

@silenius silenius deleted the updateAttributes branch October 17, 2024 13:09
@silenius silenius mentioned this pull request Oct 17, 2024
Merged
5 tasks
@silenius
Copy link
Contributor Author

FYI I've reworked this in #5738

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nperez0111 nperez0111 nperez0111 approved these changes

@bdbch bdbch Awaiting requested review from bdbch

@svenadlung svenadlung Awaiting requested review from svenadlung

Assignees

@silenius silenius

Labels
Info: Invalid The issue or pullrequest is invalid Type: Bug The issue or pullrequest is related to a bug
Projects
No open projects
Tiptap
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@silenius @nperez0111 @svilen-ivanov-kubit